Skip to content

test: add Poll::Pending spill stream coverage for async spill re-entry paths#23353

Open
pantShrey wants to merge 4 commits into
apache:mainfrom
pantShrey:test/pending_stream_smj_test
Open

test: add Poll::Pending spill stream coverage for async spill re-entry paths#23353
pantShrey wants to merge 4 commits into
apache:mainfrom
pantShrey:test/pending_stream_smj_test

Conversation

@pantShrey

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

The SortMergeJoin refactor in #22230 introduced async spill read paths in both materializing_stream.rs (poll_spilled_batches) and bitwise_stream.rs (the spilled-batch loop in process_key_match_with_filter). Both call SpillFile::read_stream() and poll the resulting byte stream, which can return Poll::Pending.

The existing spill tests exercise spilling and restoring batches, but always run against the default local-file SpillFile, whose read_stream() happens to resolve synchronously on first poll in practice. As a result, the re-entry logic in both streams has no dedicated test coverage.

What changes are included in this PR?

  • Adds a PendingSpillFile / PendingTempFileFactory pair in sort_merge_join/tests.rs that wraps the default local SpillFile backend (via DiskManagerMode::Custom). Every spill read splits the real spill file's bytes into small fixed-size chunks and yields Poll::Pending before each chunk, forcing SpillReaderStream's IPC decoder to genuinely suspend mid-read and resume from partially buffered state across multiple real Pending cycles, rather than completing on the first poll.
  • Adds materializing_spill_pending_stream, covering Inner/Left/ Right/Full joins (the BufferedBatchState::Spilled restore path in poll_spilled_batches).
  • Adds bitwise_spill_pending_stream, covering LeftSemi/LeftAnti/ RightSemi/RightAnti joins with a filter (the inner_key_spill re-entry path in process_key_match_with_filter).
  • Both tests compare the Pending-forced spilled result against a no-spill run of the same join, asserting the results are identical, so any state corruption introduced by suspending mid-read would be caught.

Are these changes tested?

Yes -- this is a PR to test recent changes

Are there any user-facing changes?

No.

@kumarUjjawal

Copy link
Copy Markdown
Contributor

@pantShrey Thank you for working on this, I will review it soon.

@kumarUjjawal kumarUjjawal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I will leave it open for now if someone else has any thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Poll::Pending spill stream test coverage for SMJ async spill paths

2 participants